Skip to content

feat(spans): Equalize name and description for manual spans#6070

Merged
loewenheim merged 10 commits into
masterfrom
sebastian/name-description-manual
Jun 12, 2026
Merged

feat(spans): Equalize name and description for manual spans#6070
loewenheim merged 10 commits into
masterfrom
sebastian/name-description-manual

Conversation

@loewenheim

@loewenheim loewenheim commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

This ensures that for spans (V1 or V2) with an origin of "manual", name and description coincide. For V1 spans, this means copying the description to the name, and the reverse for V2 spans. Other spans behave as before.

  • For backfilling the name, the new logic is inserted just before the places we would create a name based on sentry-conventions.
  • For backfilling the description, there is a new processing step backfill_description that runs right after PII scrubbing. Unfortunately it can't simply be integrated into normalize_span because it's not safe to backfill the description before PII scrubbing—the description could end up containing a URL with PII, for example. Also, in the course of this, we move the (limited) functionality that existed for backfilling the description based on attributes out of write_legacy_attributes (where it was somewhat misplaced anyway) and centralize it in normalize_sentry_description.

Future work:

  • Flesh out description backfilling logic based on sentry-conventions
  • The concern about PII noted above also applies to the name backfilling that already exists. We'll have to find a solution for that.

Closes INGEST-955.

@loewenheim loewenheim requested a review from a team as a code owner June 10, 2026 14:55
@loewenheim loewenheim self-assigned this Jun 10, 2026
Comment thread relay-spans/src/description.rs
@linear-code

linear-code Bot commented Jun 10, 2026

Copy link
Copy Markdown

INGEST-955

Comment thread relay-event-normalization/src/eap/mod.rs
Comment thread relay-event-normalization/src/eap/mod.rs
Comment thread relay-server/src/processing/transactions/spans.rs
Comment thread relay-server/src/processing/spans/process.rs Outdated
Comment thread relay-server/src/processing/spans/process.rs
Comment thread relay-server/src/processing/transactions/spans.rs
Comment thread relay-server/src/processing/spans/process.rs
Comment thread relay-spans/src/name.rs Outdated
Some(name_for_op_and_attributes(op, &AttributeGetter(attributes)))
}

fn name_for_origin_and_description(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fn name_for_origin_and_description(
fn name_from_origin_and_description(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call.

Comment thread relay-spans/src/description.rs
};

process::scrub(&mut spans, ctx);
process::backfill_description(&mut spans);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe process::normalize_derived or something similar?

Comment on lines +63 to +64
.value()
.is_some_and(|attrs| attrs.contains_key(SENTRY__DESCRIPTION))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might need a check which doesn't just use value() but also considers meta, Empty trait might be able to do that for you.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean check if attrs contains SENTRY__DESCRIPTION with a nonempty value?

Comment on lines +21 to +24
if attributes
.get_value(SENTRY__ORIGIN)
.and_then(|o| o.as_str())
== Some("manual")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This formatting is weird af

@loewenheim

Copy link
Copy Markdown
Contributor Author

@Dav1dde I've refactored the functions that copy the description over somewhat in 9a7ba1a. They now properly take Annotated into account. Not sure if it's an improvement …

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 4 total unresolved issues (including 3 from previous reviews).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 9a7ba1a. Configure here.

Comment thread relay-event-normalization/src/eap/mod.rs
Comment thread relay-event-normalization/src/eap/mod.rs Outdated
@Dav1dde

Dav1dde commented Jun 12, 2026

Copy link
Copy Markdown
Member

Not sure if it's an improvement …

Why do you think it is not an improvement?

@loewenheim

Copy link
Copy Markdown
Contributor Author

Why do you think it is not an improvement?

I mean, I believe it is in terms of correctness. But it does complicate the code a fair bit. Feels like we may be missing an abstraction for combining Annotateds.

Comment thread relay-spans/src/description.rs Outdated
Comment on lines +27 to +41
if let Some(Annotated(Some(Value::String(db_query)), meta)) =
attributes.get_annotated_value(DB__QUERY__TEXT)
{
return Some(Annotated(Some(db_query.clone()), meta.clone()));
}

if let Some(Annotated(Some(Value::String(method)), method_meta)) =
attributes.get_annotated_value(HTTP__REQUEST__METHOD)
&& let Some(Annotated(Some(Value::String(url)), url_meta)) =
attributes.get_annotated_value(URL__FULL)
{
let description = format!("{method} {url}");
let meta = method_meta.clone().merge(url_meta.clone());
return Some(Annotated(Some(description), meta));
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of actually merging the meta, I think we're fine if we just don't. We've historically never marked data which we added, as that is non-destructive and generally obvious. User/SDK doesn't set it, must be added by the Server.

We could think about adding metadata from which fields we synthesized the description if we wanted, but I am not entirely sure if that's useful.

@loewenheim loewenheim force-pushed the sebastian/name-description-manual branch from 9a7ba1a to 14ab996 Compare June 12, 2026 11:35
Comment on lines +27 to +29
if let Some(&Value::String(db_query)) = attributes.get_value(DB__QUERY__TEXT).as_ref() {
return Some(db_query.clone());
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Moving sentry.description population to after PII scrubbing bypasses custom scrubbing rules for that field, potentially leaking sensitive data from DB queries or URLs.
Severity: HIGH

Suggested Fix

Ensure that derived span descriptions are generated before the PII scrubbing process runs. This would involve calling normalize_derived() before process::scrub(), restoring the previous behavior where custom PII rules targeting sentry.description can effectively remove sensitive information.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: relay-spans/src/description.rs#L27-L29

Potential issue: For non-manual V2 spans, such as database or HTTP spans, the
`sentry.description` attribute is now populated after PII scrubbing has already run.
Previously, this attribute was populated before scrubbing, allowing custom PII rules
targeting `sentry.description` to remove sensitive data like database query text or URL
parameters. Because the attribute is now absent during the scrubbing process, these
custom rules are no longer effective. This results in potentially sensitive information,
which was previously scrubbed, being included in the `sentry.description` field.

@loewenheim loewenheim enabled auto-merge June 12, 2026 12:12
@loewenheim loewenheim added this pull request to the merge queue Jun 12, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 12, 2026
@loewenheim loewenheim added this pull request to the merge queue Jun 12, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 12, 2026
@loewenheim loewenheim added this pull request to the merge queue Jun 12, 2026
Merged via the queue into master with commit fa88a08 Jun 12, 2026
33 checks passed
@loewenheim loewenheim deleted the sebastian/name-description-manual branch June 12, 2026 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants